fix(storage): BidiAppendableUpload Takeover operation fixes#13776
Conversation
Dhriti07
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates state transitions and retry logic in BidiUploadState.java, and enables the PROD backend for ITAppendableUploadTest. It also refactors the awaitState and awaitAck methods to move the await call into the loop body. The feedback suggests increasing the short 5ms timeout in these loops to mitigate potential high CPU usage and performance degradation caused by frequent polling.
| while (!states.contains(this.state)) { | ||
| if (resultFuture.isDone()) { | ||
| return; | ||
| } | ||
| stateUpdated.await(5, TimeUnit.MILLISECONDS); | ||
| } |
There was a problem hiding this comment.
Using a very short timeout of 5 milliseconds for await in a loop results in frequent waking up and lock acquisition (polling), which can lead to high CPU usage and performance degradation under load.
If resultFuture completion signals stateUpdated (or if you can add a listener to resultFuture to signal it upon completion), you can use a simple, indefinite stateUpdated.await() without a timeout. If a timeout is absolutely necessary as a fallback, consider increasing it to a more reasonable value (e.g., 100ms or 1s) to reduce CPU overhead.
| while (!states.contains(this.state)) { | |
| if (resultFuture.isDone()) { | |
| return; | |
| } | |
| stateUpdated.await(5, TimeUnit.MILLISECONDS); | |
| } | |
| while (!states.contains(this.state)) { | |
| if (resultFuture.isDone()) { | |
| return; | |
| } | |
| stateUpdated.await(100, TimeUnit.MILLISECONDS); | |
| } |
| while (confirmedBytes < writeOffset) { | ||
| if (resultFuture.isDone()) { | ||
| return; | ||
| } | ||
| confirmedBytesUpdated.await(5, TimeUnit.MILLISECONDS); | ||
| } |
There was a problem hiding this comment.
Using a very short timeout of 5 milliseconds for await in a loop results in frequent waking up and lock acquisition (polling), which can lead to high CPU usage and performance degradation under load.
If resultFuture completion signals confirmedBytesUpdated (or if you can add a listener to resultFuture to signal it upon completion), you can use a simple, indefinite confirmedBytesUpdated.await() without a timeout. If a timeout is absolutely necessary as a fallback, consider increasing it to a more reasonable value (e.g., 100ms or 1s) to reduce CPU overhead.
| while (confirmedBytes < writeOffset) { | |
| if (resultFuture.isDone()) { | |
| return; | |
| } | |
| confirmedBytesUpdated.await(5, TimeUnit.MILLISECONDS); | |
| } | |
| while (confirmedBytes < writeOffset) { | |
| if (resultFuture.isDone()) { | |
| return; | |
| } | |
| confirmedBytesUpdated.await(100, TimeUnit.MILLISECONDS); | |
| } |
| @@ -544,6 +544,7 @@ final void updateStateFromResponse(BidiWriteObjectResponse response) { | |||
| if (state == State.INITIALIZING || state == State.RETRYING) { | |||
There was a problem hiding this comment.
nit: Fix typo in PR message.
Dhriti07
changed the title
2 participants
BidiAppendableUpload Takeover operation fixes